Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

USB Host UVC driver version 2 #84

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

tore-espressif
Copy link
Collaborator

@tore-espressif tore-espressif commented Nov 5, 2024

Introduction

This PR introduces version 2 of the USB Host UVC driver, aimed at enabling ESP SoCs to stream frames from connected USB cameras.

The driver provides frames to the user via a callback, offering a flexible interface that can serve as a foundation for more advanced frame processing applications.

Internally, the driver manages a FreeRTOS Queue containing multiple frame buffers, which helps prevent buffer overflows and underflows during streaming.

Detailed feature descriptions, future plans, and usage instructions are available in the README.md file.

Information for Testers

This section provides instructions for testing the driver’s functionality and public API.

Two examples are included for testing:

1. basic_uvc_stream

  • Test Setup: Tested on ESP32-P4 with a customer-specified dual-camera setup.
  • Functionality: Opens two UVC streams and continuously starts and stops them.

2. camera_display

  • Test Setup: Tested on ESP32-S3 with multiple compatible cameras.
  • Functionality: Initializes a display, opens a single UVC stream, decodes received JPEG frames, and displays them on the screen.

Information for Reviewers

This section provides guidance for those reviewing the internal code structure of the driver.

The driver includes a host_test folder with tests designed to run on Linux. These tests use a mocked version of the esp-idf/usb component, focusing on the following areas, which should need less in-depth review:

  • Configuration descriptor parsinguvc_descriptor_parsing.c
  • USB transfer callbacksuvc_isoc.c and uvc_bulk.c

The following file, focused on human-readable output, also requires only a light review:

  • Descriptor printinguvc_descriptor_printing.c

Areas not yet covered by host_tests and requiring more detailed review include:

  • Frame buffer managementuvc_frame.c (manages queue of empty frame buffers)
  • Control requestsuvc_control.c (handles video format negotiation and other control requests)
  • Core driver functionalityuvc_host.c (includes driver installation, device management, stream initiation, etc.)

Related Information

@tore-espressif tore-espressif self-assigned this Nov 5, 2024
@tore-espressif tore-espressif marked this pull request as draft November 5, 2024 17:12
@tore-espressif tore-espressif force-pushed the feature/uvc_2 branch 6 times, most recently from 7adb8ec to e9a9097 Compare November 12, 2024 09:55
@tore-espressif tore-espressif marked this pull request as ready for review November 12, 2024 15:37
Copy link
Collaborator Author

@tore-espressif tore-espressif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peter-marcisovsky Thank you very much for such a thorough review!

I fixed most of the things, answered other and left a few for later.

I left the changes in separate comment for now, so it is easier to review

Copy link
Collaborator

@roma-jam roma-jam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Tomas,

I did the first round and it works! Cool!

Regarding the changes, I did check the:

  • READMEs and Description
  • Folder struct and APIs

Couple of notes I've already put, but to proceed, I think that I might meed some additional information.
Like requirements. I prepared the list of the questions, answers to them will help me understand what should I check and how.

  1. What are the targeting versions of esp-idf for examples? (I've tried the basic_uvc_stream on v5.2 and there is fatal error: sd_pwr_ctrl_by_on_chip_ldo.h: No such file or directory 😐
  2. What are the requirements for the driver? Should it notify the user while the device is attached to the ESP32 and then user should start streaming? Or it just provides the possibility to open the stream, when the device is already connected and user knows the vid/pid? More like the general idea. Using with hubs, several streams on one device or not, hot detaching (detaching cameras during streaming) and so on
  3. Single-thread/Multi-thread. Does it support opening several streams on one physical device from several different tasks?

I have checked the docs/ folder, thanks for the png! (also, if we need to provide special additional information, such as "create" above the arrow, maybe we need to think about renaming the name of the API)

I will proceed with the review, feel free to address current comments.

host/class/uvc/usb_host_uvc_2/README.md Outdated Show resolved Hide resolved
host/class/uvc/usb_host_uvc_2/README.md Outdated Show resolved Hide resolved
host/class/uvc/usb_host_uvc_2/include/usb/uvc_host.h Outdated Show resolved Hide resolved
enum uvc_host_dev_event {
UVC_HOST_ERROR, /**< USB transfer error */
UVC_HOST_DEVICE_DISCONNECTED, /**< Device was suddenly disconnected. The stream is stopped. */
UVC_HOST_FRAME_BUFFER_OVERFLOW, /**< The received frame was discarded because it exceeded the available frame buffer space.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that also an error? Or it is possible to increase the frame_size right after getting this event?

If this is an error, maybe it could be better:

Suggested change
UVC_HOST_FRAME_BUFFER_OVERFLOW, /**< The received frame was discarded because it exceeded the available frame buffer space.
UVC_HOST_ERROR_FRAME_BUFFER_OVERFLOW, /**< The received frame was discarded because it exceeded the available frame buffer space.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The frame buffer size cannot be changed of the UVC stream was opened (I can add this into TODO list)

It's not a real error because the stream continues. It just informs the user that a frame was dropped. If this happens too often, the user can closed the stream and open it again with larger frame buffers...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then maybe?

Suggested change
UVC_HOST_FRAME_BUFFER_OVERFLOW, /**< The received frame was discarded because it exceeded the available frame buffer space.
UVC_HOST_DROPPED_FRAMES, /**< The received frame was discarded because it exceeded the available frame buffer space.

host/class/uvc/usb_host_uvc_2/include/usb/uvc_host.h Outdated Show resolved Hide resolved
* - ESP_INVALID_ARG: frame or stream_hdl is NULL
* - ESP_FAIL: The frame was not returned to the driver
*/
esp_err_t uvc_host_frame_return(uvc_host_stream_hdl_t stream_hdl, uvc_host_frame_t *frame);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the logic, described above, maybe it would be cleaner to name it something like:

Suggested change
esp_err_t uvc_host_frame_return(uvc_host_stream_hdl_t stream_hdl, uvc_host_frame_t *frame);
esp_err_t uvc_host_frame_process_complete(uvc_host_stream_hdl_t stream_hdl, uvc_host_frame_t *frame);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is that possible to avoid returning value in the uvc_host_frame_callback_t and do that via the uvc_host_frame_process_complete() call?

If the frame is handled right in the callback, it is possible to call it right from there.
If the frame is not handled in the callback, it will be called someplace else.

WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This naming was taken from esp32-camera. I'll check if it can be made better

host/class/uvc/usb_host_uvc_2/uvc_bulk.c Outdated Show resolved Hide resolved
@tore-espressif tore-espressif force-pushed the feature/uvc_2 branch 3 times, most recently from 430f7d0 to f2e7547 Compare November 25, 2024 14:22
Copy link
Collaborator

@roma-jam roma-jam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, couple of new comments, but nothing serious.
I will try to verify it on a hardware, should I have some special camera or anything I should know before testing?

Also, did you do any measurements to what extend it is better than the implementation, based on libusb?

UPD: Is our current example in esp-idf is compatible with this new class driver?

host/class/uvc/usb_host_uvc_2/uvc_host.c Outdated Show resolved Hide resolved

// @todo create a function that will wait for all frames to be returned
if (!uvc_frame_are_all_returned(uvc_stream)) {
vTaskDelay(pdMS_TO_TICKS(70)); // Wait 70ms so the user can return all frames
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems, that this is the right place to add a mutex here....but you already have a TODO here )

xSemaphoreGive((SemaphoreHandle_t)transfer->context);
}

esp_err_t uvc_host_usb_ctrl(uvc_host_stream_hdl_t stream_hdl, uint8_t bmRequestType, uint8_t bRequest, uint16_t wValue, uint16_t wIndex, uint16_t wLength, uint8_t *data)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we plan to support something else rather then usb?
If no, maybe usb in not necessary to be in the name.

Suggested change
esp_err_t uvc_host_usb_ctrl(uvc_host_stream_hdl_t stream_hdl, uint8_t bmRequestType, uint8_t bRequest, uint16_t wValue, uint16_t wIndex, uint16_t wLength, uint8_t *data)
esp_err_t uvc_host_ctrl(uvc_host_stream_hdl_t stream_hdl, uint8_t bmRequestType, uint8_t bRequest, uint16_t wValue, uint16_t wIndex, uint16_t wLength, uint8_t *data)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I wanted to emphasize that it is not 'Camera control' but 'USB Control' request.

I'll go through all function names again and revise it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then "ctrl_request" probably would be a bit better option....

Suggested change
esp_err_t uvc_host_usb_ctrl(uvc_host_stream_hdl_t stream_hdl, uint8_t bmRequestType, uint8_t bRequest, uint16_t wValue, uint16_t wIndex, uint16_t wLength, uint8_t *data)
esp_err_t uvc_host_ctrl_request(uvc_host_stream_hdl_t stream_hdl, uint8_t bmRequestType, uint8_t bRequest, uint16_t wValue, uint16_t wIndex, uint16_t wLength, uint8_t *data)

#include "usb/uvc_host.h"
#include "uvc_control.h"
#include "uvc_stream.h"
#include "uvc_types_priv.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need _priv.h postfix in the filenames?
They are already in the private folder. Or am I missing something?

I mean, I just bumped in that when I tried to find the file, related to "uvc_frame.c" which is usually has the same name, but header. I didn't find it and then I realized that I need to search "_frame_priv.h" )
Just a bit confusing IMHO.

But maybe there is an idea behind such naming.

@roma-jam
Copy link
Collaborator

roma-jam commented Nov 27, 2024

@tore-espressif
Sorry, I'm stuck trying to claim open a stream from my camera on S3.
Currently, I'm trying to understand what did I do wrong:

image

Will proceed with review shortly...

@tore-espressif
Copy link
Collaborator Author

@tore-espressif Sorry, I'm stuck trying to claim open a stream from my camera on S3. Currently, I'm trying to understand what did I do wrong:

image

Will proceed with review shortly...

@roma-jam that is really weird, Can you share full CFG descriptor?

@roma-jam
Copy link
Collaborator

roma-jam commented Nov 27, 2024

USB 2.0 Camera
Sonix Technology Co., Ltd.
CNE-CWC2

Seems like we have it in host_test, as CANYON_CNE_CWC2. Which probably means that I do smth wrong.

UPD: I open the stream on the same device again. But the stream was already opened and interface was claimed, so I don't get any error on that and driver tries to claim the interface one more time. But it is already claimed, that is why the EP already allocated. Good.

09 02 7D 02 04 01 00 80 FA 08 0B 00 02 0E 03 00
05 09 04 00 00 01 0E 01 00 05 0D 24 01 00 01 4D
00 C0 E1 E4 00 01 01 09 24 03 02 01 01 00 04 00
1A 24 06 04 70 33 F0 28 11 63 2E 4A BA 2C 68 90
EB 33 40 16 08 01 03 01 0F 00 12 24 02 01 01 02
00 00 00 00 00 00 00 00 03 0E 20 00 0B 24 05 03
01 00 00 02 3F 04 00 07 05 83 03 10 00 06 05 25
03 10 00 09 04 01 00 00 0E 02 00 05 0E 24 01 01
33 01 81 00 02 02 01 01 01 00 0B 24 06 01 05 00
01 00 00 00 00 32 24 07 01 00 80 02 E0 01 00 00
77 01 00 00 CA 08 00 60 09 00 15 16 05 00 06 15
16 05 00 80 1A 06 00 20 A1 07 00 2A 2C 0A 00 40
42 0F 00 80 84 1E 00 32 24 07 02 00 60 01 20 01
00 C0 7B 00 00 80 E6 02 00 18 03 00 15 16 05 00
06 15 16 05 00 80 1A 06 00 20 A1 07 00 2A 2C 0A
00 40 42 0F 00 80 84 1E 00 32 24 07 03 00 40 01
F0 00 00 C0 5D 00 00 80 32 02 00 58 02 00 15 16
05 00 06 15 16 05 00 80 1A 06 00 20 A1 07 00 2A
2C 0A 00 40 42 0F 00 80 84 1E 00 32 24 07 04 00
B0 00 90 00 00 F0 1E 00 00 A0 B9 00 00 C6 00 00
15 16 05 00 06 15 16 05 00 80 1A 06 00 20 A1 07
00 2A 2C 0A 00 40 42 0F 00 80 84 1E 00 32 24 07
05 00 A0 00 78 00 00 70 17 00 00 A0 8C 00 00 96
00 00 15 16 05 00 06 15 16 05 00 80 1A 06 00 20
A1 07 00 2A 2C 0A 00 40 42 0F 00 80 84 1E 00 1A
24 03 00 05 80 02 E0 01 60 01 20 01 40 01 F0 00
B0 00 90 00 A0 00 78 00 00 06 24 0D 01 01 04 09
04 01 01 01 0E 02 00 00 07 05 81 05 80 00 01 09
04 01 02 01 0E 02 00 00 07 05 81 05 00 01 01 09
04 01 03 01 0E 02 00 00 07 05 81 05 00 02 01 09
04 01 04 01 0E 02 00 00 07 05 81 05 58 02 01 09
04 01 05 01 0E 02 00 00 07 05 81 05 20 03 01 09
04 01 06 01 0E 02 00 00 07 05 81 05 BC 03 01 08
0B 02 02 01 00 00 04 09 04 02 00 00 01 01 00 04
09 24 01 00 01 29 00 01 03 0C 24 02 01 01 02 00
01 00 00 00 00 0B 24 06 02 01 02 01 00 02 00 00
09 24 03 03 01 01 00 02 00 09 04 03 00 00 01 02
00 00 09 04 03 01 01 01 02 00 00 07 24 01 03 01
01 00 0B 24 02 01 01 02 10 01 80 3E 00 09 05 84
05 20 00 04 00 00 07 25 01 01 00 00 00

@roma-jam
Copy link
Collaborator

roma-jam commented Nov 27, 2024

Same board, same camera. Different drivers.

UVC:
image
UVC_2:
image

Why the initial sequence of Get Curr/Set Curr/Get Max/Get Min is so different?

UPD: Also, I can't get why the UVC set Iface = 1 and Alt = 3, whether UVC_2 set Iface=1 and Alt =4.

Any idea?

@roma-jam
Copy link
Collaborator

Yep, I opened the stream, but I keep getting the following error:
ESP_LOGW(TAG, "usb err %d", isoc_desc->status); with USB_TRANSFER_STATUS_ERROR

and I still have no idea why. In process.


bool return_frame = true; // Default to returning the frame in case streaming has been stopped
if (invoke_fb_callback) {
memcpy((uvc_host_stream_format_t *)&this_frame->vs_format, &uvc_stream->constant.vs_format, sizeof(uvc_host_stream_format_t));

Check warning

Code scanning / clang-tidy

Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling] Warning

Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]

vs_control->bFormatIndex = format_desc->bFormatIndex;
vs_control->bFrameIndex = frame_desc->bFrameIndex;
vs_control->dwFrameInterval = UVC_DESC_FPS_TO_DWFRAMEINTERVAL(vs_format->fps); // Implicit conversion from float to uint32_t

Check warning

Code scanning / clang-tidy

The left operand of '!=' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult] Warning

The left operand of '!=' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult]

static inline bool uvc_is_vs_format_equal(const uvc_host_stream_format_t *a, const uvc_host_stream_format_t *b)
{
if (a->h_res == b->h_res &&

Check warning

Code scanning / clang-tidy

The left operand of '==' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult] Warning

The left operand of '==' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult]
host/class/uvc/usb_host_uvc/uvc_control.c Dismissed Show dismissed Hide dismissed

// Pass the result to user
if (vs_result_ret) {
memcpy(vs_result_ret, &vs_result, sizeof(uvc_vs_ctrl_t));

Check warning

Code scanning / clang-tidy

Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling] Warning

Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
UVC_CHECK(frame && data, ESP_ERR_INVALID_ARG);
UVC_CHECK(frame->data_len + data_len <= frame->data_buffer_len, ESP_ERR_INVALID_SIZE);

memcpy(frame->data + frame->data_len, data, data_len);

Check warning

Code scanning / clang-tidy

Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling] Warning

Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
err, TAG,);

// Save info
memcpy((uvc_host_stream_format_t *)&uvc_stream->constant.vs_format, &stream_config->vs_format, sizeof(uvc_host_stream_format_t));

Check warning

Code scanning / clang-tidy

Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling] Warning

Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
// For IN transfers we must transfer data ownership to the driver
const bool in_transfer = bmRequestType & USB_BM_REQUEST_TYPE_DIR_IN;
if (!in_transfer) {
memcpy(start_of_data, data, wLength);

Check warning

Code scanning / clang-tidy

Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling] Warning

Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]

// For OUT transfers, we must transfer data ownership to user
if (in_transfer) {
memcpy(data, start_of_data, wLength);

Check warning

Code scanning / clang-tidy

Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling] Warning

Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
UVC_EXIT_CRITICAL();

if (invoke_fb_callback) {
memcpy((uvc_host_stream_format_t *)&this_frame->vs_format, &uvc_stream->constant.vs_format, sizeof(uvc_host_stream_format_t));

Check warning

Code scanning / clang-tidy

Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling] Warning

Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
This driver does not use libuvc anymore,
it is native to Espressif's USB Host Library
@tore-espressif
Copy link
Collaborator Author

The last commit adds the following:

  • Sudden disconnection reaction improved (no more transfers are submitted to the disconnected device)
  • Allowed external events handling (similar to MSC or HID class)
  • uvc_host_stream_event_data_t now follow open-closed principle: Open for extension, closed for modification

Overall, this is Release candidate for v2. Other features and bugs will be addressed in bugfix releases

@roma-jam roma-jam self-requested a review December 11, 2024 21:09
@leeebo leeebo self-requested a review December 12, 2024 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants